replace ClientThreadHandler "Debug" logs with NonSessionLog #830
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A long writeup of what I did to rewrite the Debug logging
Preface
I'm not super happy with how this came out, but it could be worse. I wish it was more elegant, but this is the best I could figure out.
The situation
The entire QF/n log mechanism is session-based. To log a message, the code block needs to get the log handle from a reference to the session. Unfortunately, there are a few spots where we want to log, but the block of code is not directly associated with a session. There just isn't a natural method for logging in this situation, and never has been.
What had existed
For over a decade, we had an ugly hack in there that would write a "ClientHandlerThread-....-log" file every time a ClientHandlerThread was instantiated. It was originally added to help debug some socket setup stuff, but was never removed or improved. As the project grew, a few other places used it as well. In mostly only triggered for Acceptors, and after a few weeks of operation, developers of such apps would find hundreds of ClientHandlerThread logs in their log directory. It was horrible, and lots of folks rightly hated it.
My goal
I wanted to add a non-session-specific log that would:
What I came up with
I created a NonSessionLog class which takes the app's instantiated ILogFactory in its constructor, so that it can use the settings of that factory and write to the same place. Unfortunately, I had to extend the ILogFactory interface by adding method CreateNonSessionLog(), so anyone with a custom LogFactory will need to implement that method... but luckily that's not very difficult (see FileLogFactory for a good example)!
The commit that specifically adds the NonSessionLog is e9abea3.
There are some refactoring/cleanup commits in this PR also, but they are standalone.
A design crossroads
To implement NonSessionLog, I waffled over whether:
I ended up going with the former, which, while more annoying to write, seemed like the more-maintainable/less-"surprising" way to go.
Unfortunate side-effects (i.e. what I hate)
Non-Session-Log.events.log
, but when it does, it will create an always-emptyNon-Session-Log.messages.log
also. I couldn't change this without further altering the ILog/ILogFactory interfaces. (I may revisit this in the future.)Old comment that I wrote after the first commit (07d2d89):
This is phase 1: make it stop creating the Debug
log, and just generally getting my head around
the flow of the deep transport logic that writes
to this log.
(Current debug-log calls are either eliminated
or temporarily routed to console-writes)
The reason these log lines are weird is because
they happen in code that isn't specific to a
session, so it can't follow the existing paradigm
(where each session has its own logger).
But some of those logs are still valuable,
so phase 2 will be logging them in an alternate
way, likely in a way that will only create
the logs when absolutely necessary (as opposed
to the current impl which can create a ton
of empty logs).